Skip to content

fix(auth): unify USP login check so Remote Assistance can open Wi-Fi/Internet Settings (#1119)#1122

Open
AustinChangLinksys wants to merge 5 commits into
dev-2.6.0from
fix/ra-wifi-internet-auth-1119
Open

fix(auth): unify USP login check so Remote Assistance can open Wi-Fi/Internet Settings (#1119)#1122
AustinChangLinksys wants to merge 5 commits into
dev-2.6.0from
fix/ra-wifi-internet-auth-1119

Conversation

@AustinChangLinksys

Copy link
Copy Markdown
Collaborator

Summary

Fixes #1119 — in Remote Assistance (RA) mode, opening Wi-Fi Settings (and Internet Settings) only showed "You are not signed in. Please sign in and try again.", while the same pages work in local Linksys Now.

Root cause

RA sessions are pre-authorized via authToken (the login handshake is skipped), so the WASM client's usp.isAuthenticated stays false by design. The Wi-Fi and Internet Settings providers gated their fetch on that raw transport flag and short-circuited before fetching — even though the data layer succeeds in RA (the attached log shows [USP][WifiData]: Fetched — clients: 1, radios: 2).

These two gates were both redundant and wrong:

  • The router already guards every /usp route on loginType (RA-aware) — the page is unreachable when logged out.
  • 9 sibling feature providers (dmz, local_network, firewall, port_forwarding, static_routing, ipv6_port_service, dhcp, instant_safety, wifi_advanced) have no such gate and work correctly, including in RA.

Changes

Single source of truth for login state (previously none existed):

  • AuthState.isRemoteAssistance — canonical login-intent check, replacing scattered inline loginType == LoginType.remote comparisons (dashboard orchestrator, router, RA session guard).
  • uspAuthReadyProvider — single source of truth for "USP layer is authorized to serve data" (isRemoteAssistance || usp.isAuthenticated). Documents that usp.isAuthenticated is a non-reactive plain getter.

The fix:

  • Remove the raw isAuthenticated fetch gate in the Wi-Fi and Internet Settings providers (keep the usp == null guard; the service provider also throws ServiceNotInitializedError when USP is unavailable).
  • Converge dashboard_orchestrator, router_provider and the RA session guard onto the new getter. The orchestrator keeps its direct usp.isAuthenticated reads because it performs restoreSession and must observe the flag flip synchronously (a cached provider read would be stale).

Tests

Verification: flutter analyze clean on all changed files; full functional suite (./run_tests.sh) — 3269 passed.

Follow-up (out of scope)

session_service.dart:68 and sse_bootstrap (sse_providers.dart:165) still read the raw flag but are off the RA render path:

  • session_service is a ref-less service on the login/recovery path; making it RA-aware requires a signature change and RA-recovery verification.
  • sse_bootstrap — RA already gets SSE via the orchestrator's connect(); changing the gate risks a double-connect race and needs a SseConnectionManager idempotency review.

…Internet Settings (#1119)

In Remote Assistance mode the WASM USP client is pre-authorized via
authToken, so `usp.isAuthenticated` stays false by design. The Wi-Fi and
Internet Settings providers gated their fetch on that raw transport flag,
short-circuiting with "You are not signed in" even though the backend
serves data fine. The router already guards all /usp routes on loginType
(RA-aware) and 9 sibling feature providers have no such gate, making these
two gates both redundant and wrong.

- Add `AuthState.isRemoteAssistance` as the canonical login-intent check,
  replacing scattered `loginType == LoginType.remote` comparisons.
- Add `uspAuthReadyProvider` as the single source of truth for
  "USP layer is authorized to serve data" (RA || usp.isAuthenticated),
  documenting that usp.isAuthenticated is non-reactive.
- Remove the raw isAuthenticated fetch gate in the Wi-Fi and Internet
  Settings providers (keep the usp == null guard). This fixes #1119.
- Converge dashboard_orchestrator, router_provider and the RA session
  guard onto the new getter; the orchestrator keeps direct
  usp.isAuthenticated reads because it observes restoreSession flips.
- Tests: repurpose the internet "unauthenticated" test into an RA-bypass
  proof, add wifi RA-bypass + usp==null tests, and add unit tests for the
  new getter and provider.

Follow-up (out of scope): session_service.dart and sse_bootstrap still
read the raw flag but are off the RA render path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@AustinChangLinksys AustinChangLinksys left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated Review — Round 1 · 69eb8f1..a79aded (full)

Verdict: 💬 Self-review (comment only) — Austin's own PR; GitHub prohibits self-approve. Full review for reference.

Conf. Where Issue (one-liner)
⚠️ 🟢 High lib/providers/auth/usp_auth_ready_provider.dart:19 [both reviewers] New provider has zero production consumers (dead code / future infra)
⚠️ 🟡 Med lib/page/internet_settings/providers/usp_internet_settings_notifier.dart:139 performSave() has no RA-mode guard; removed fetch gate was the only write-path pre-check
⚠️ 🟡 Med lib/page/wifi_settings/providers/usp_wifi_settings_provider.dart:174 performSave() same issue — isRemoteReadOnly not wired up in settings views
⚠️ 🟡 Med lib/route/router_provider.dart:162-164 loginType used directly in local-auth path, isRemoteAssistance migration is partial
💡 🟢 High lib/page/wifi_settings/providers/usp_wifi_settings_provider.dart diff [both reviewers] Comment block has 6-space indent on one line vs 4-space on others
💡 🟡 Med lib/page/dashboard/orchestrator/dashboard_orchestrator.dart:94-100 ref.listen only invalidates on LoginType.local transition, not on RA login
💡 🟡 Med lib/providers/auth/usp_auth_ready_provider.dart:14-18 Staleness caveat documented but no lint/API-shape enforcement against ref.watch misuse
💡 🟡 Med lib/page/remote_assistance/views/remote_assistance_session_guard.dart:37,64 Dual RA guards use different mechanisms — semantics unclear

Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
Items marked [both reviewers] were independently flagged by two agents → higher confidence.

⚠️ Warning Details

W-1 — uspAuthReadyProvider has no production consumer 🟢 High [both reviewers]

lib/providers/auth/usp_auth_ready_provider.dart:19

The provider is introduced as the "single source of truth" for USP auth readiness, but searching lib/ in the PR branch finds it consumed only within its own file and one explanatory comment in dashboard_orchestrator.dart (which explicitly bypasses it due to the staleness caveat). Neither the WiFi nor Internet settings notifier references it. If this is intentional future infrastructure, the PR description should say so.

Fix: Either note "reserved for follow-up" in the PR description, or wire it into the notifiers if the staleness caveat does not apply to those call sites.


W-2 & W-3 — performSave() has no RA-mode write guard 🟡 Medium [single reviewer A]

lib/page/internet_settings/providers/usp_internet_settings_notifier.dart:139
lib/page/wifi_settings/providers/usp_wifi_settings_provider.dart:174

Before this PR, the if (!usp.isAuthenticated) { throw ConnectivityError … } gate in performFetch() also prevented RA sessions from ever reaching edit state — so performSave() was unreachable in practice. Now that performFetch() succeeds in RA mode, a user can enter edit mode and trigger performSave(). Neither settings view wires up remoteAccessProvider.select(s => s.isRemoteReadOnly) (the recommended pattern from docs/plans/2026-01-20-remote-read-only-mode-usage.md), and USP SET calls go through the WASM client path — not through RouterRepository where JNAP write-blocking lives.

// usp_internet_settings_notifier.dart:139 (PR branch) — no RA guard
@override
Future<void> performSave() async {
  // proceeds directly to service.saveAll() via uspMutationLockProvider.withLock()
  await ref.read(uspMutationLockProvider).withLock(() async {
    await service.saveAll(state.original, state.edited, ...);
  });
}

Whether the backend enforces RA write restrictions on USP SET is not verifiable from source; the PR assumes server-side enforcement without a client-side safety net.

Fix: Add an RA guard to both performSave() implementations, e.g.:

final isRA = ref.read(authProvider).value?.isRemoteAssistance ?? false;
if (isRA) throw const UnexpectedError(detail: 'Write not allowed in RA mode');

Or wire up isRemoteReadOnly in the settings views to disable save controls in RA mode.


W-4 — loginType migration is partial in router_provider.dart 🟡 Medium [single reviewer B]

lib/route/router_provider.dart:162-164

The PR updates the Remote-build-mode RA guard (lines 143-145) to isRemoteAssistance, but the local-auth loginType reads on lines 162-164 remain. This is pre-existing and not a regression, but the migration described in the PR description ("canonical replacement") is incomplete.

Fix: Follow-up cleanup or note in PR description.

✅ What looks good
  • Core fix is correct: usp.isAuthenticated is a WASM transport signal that stays false in RA mode by design; removing it from performFetch() unblocks #1119 without affecting local login paths.
  • AuthState.isRemoteAssistance getter: Well-documented, correctly derived, replaces inline loginType == LoginType.remote in all the changed callsites (session guard, router redirect, dashboard orchestrator).
  • uspAuthReadyProvider design logic: Sound; the staleness caveat is clearly documented. The deliberate bypass in dashboard_orchestrator (which calls restoreSession and observes the flag synchronously) is architecturally correct and well-commented.
  • Test coverage: usp_auth_ready_provider_test.dart (5 cases), RA-bypass regression tests for both notifiers, isRemoteAssistance unit test, null-USP error path — all confirmed present in the PR branch and well-structured.
  • uspMutationLockProvider.withLock() discipline preserved in both performSave() implementations.
  • No hardcoded secrets or injection vectors found.

Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.

…r concern (PR #1122 review W-1)

The provider was introduced as a "single source of truth" for USP auth
readiness but ended up with zero production consumers: wifi/internet fixed
#1119 by removing their fetch gate (relying on the router guard) rather than
routing through the provider, so it was dead code.

Keeping it would also push the auth concern back into page providers (each
page would ref.read it), which is the coupling we want to avoid. Auth is a
navigation-layer concern: the router already gates every /usp route on
loginType (RA-aware), so pages render data without an auth dependency — matching
the 9 sibling USP settings providers that never had a gate.

- Delete usp_auth_ready_provider.dart and its test.
- Drop the stale provider reference from the orchestrator comment.
- Keep AuthState.isRemoteAssistance: it is consumed by the router (the auth
  boundary) and by RA-strategy decisions in the orchestrator/session guard,
  not as a per-page login gate.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@AustinChangLinksys AustinChangLinksys left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated Review — Round 2 · 69eb8f1..a79aded (full)

Verdict: 💬 Self-review (comment only) — Austin's own PR; GitHub prohibits self-approve. Full review for reference.

Note: Prior Round 1 review was posted at the same SHA. This round re-confirms still-open items and adds new findings.

Conf. Where Issue (one-liner)
⚠️ High lib/page/internet_settings/providers/usp_internet_settings_notifier.dart:135 [both reviewers, prior] performSave() has no RA-mode write guard — still unfixed
⚠️ High lib/page/wifi_settings/providers/usp_wifi_settings_provider.dart:176 [both reviewers, prior] performSave() same — still unfixed
⚠️ High lib/providers/auth/usp_auth_ready_provider.dart:19 [both reviewers, prior] uspAuthReadyProvider has zero production consumers (dead code) — still unfixed
⚠️ High lib/route/router_provider.dart:143-145 [both reviewers] Remote-build-mode branch still uses raw loginType == LoginType.remote (not migrated to isRemoteAssistance)
⚠️ High lib/providers/auth/usp_auth_ready_provider.dart:5-24 [both reviewers] API design contradiction: documented as SSOT but explicitly bypassed in orchestrator due to non-reactive cache
⚠️ High lib/page/remote_assistance/views/remote_assistance_session_guard.dart:63 RA guard migration in _checkAndRestoreSession has no test coverage (sole migrated callsite without a test)
⚠️ Med lib/page/wifi_settings/providers/usp_wifi_settings_provider.dart:17-18 uspWifiSettingsServiceProvider force-unwraps uspClientProvider — throws unmanaged Null check operator exception on null
⚠️ High lib/providers/auth/_auth.dart:1-2 New usp_auth_ready_provider.dart not exported from barrel file
💡 Med lib/page/internet_settings/providers/usp_internet_settings_notifier.dart:66 Low-probability race: performFetch auth-gate removed; if provider builds before router redirect completes, USP call proceeds in unguarded state
💡 Med lib/route/router_provider.dart:222 Pre-existing tech debt: ref.watch in redirectLogic's ChangeNotifier method

Confidence: High = code-verified · Med = located + reasoned, not fully confirmed · Low = speculative, please double-check.
Items marked [both reviewers] were independently flagged by two agents → higher confidence.

⚠️ Warning Details

W-1 — performSave() no RA-mode write guard (Internet Settings) — High [both reviewers, prior — still unfixed]

lib/page/internet_settings/providers/usp_internet_settings_notifier.dart:135

Before this PR the if (!usp.isAuthenticated) gate in performFetch effectively blocked RA users from reaching edit state. Now that performFetch succeeds in RA mode, a user can enter edit mode and call performSave(). The notifier does not check isRemoteAssistance / isRemoteReadOnly before proceeding to service.saveAll(...). isRemoteReadOnly does not exist anywhere in the codebase.

Fix: Add an RA guard in performSave():

final isRA = ref.read(authProvider).value?.isRemoteAssistance ?? false;
if (isRA) throw const UnexpectedError(detail: 'Write not allowed in RA mode');

W-2 — performSave() no RA-mode write guard (Wi-Fi Settings) — High [both reviewers, prior — still unfixed]

lib/page/wifi_settings/providers/usp_wifi_settings_provider.dart:176

Same issue as W-1 for the Wi-Fi notifier. RA users can trigger saveQuickSetup/saveAdvanced via USP SET, changing router WiFi settings without local-admin authorization.

Fix: Same pattern — guard with isRemoteAssistance before issuing writes.


W-3 — uspAuthReadyProvider has zero production consumers — High [both reviewers, prior — still unfixed]

lib/providers/auth/usp_auth_ready_provider.dart:19

Global search confirms the only references to uspAuthReadyProvider are its definition and a comment in dashboard_orchestrator.dart explaining why not to use it. It is dead code.

Fix: Either wire it into the notifiers' fetch guards (replacing the removed isAuthenticated gate), or document clearly that it is "reserved for future use" and mark it accordingly.


W-4 — router_provider.dart:143-145 Remote-build-mode branch not migrated — High [both reviewers]

lib/route/router_provider.dart:143-145

// L143-145 (unchanged in PR)
final loginType =
    ref.read(authProvider.select((value) => value.value?.loginType));
if (loginType == LoginType.remote) {

The PR migrated L162-164 to isRemoteAssistance but left the Remote-build-mode branch at L143-145 on the raw loginType == LoginType.remote pattern. Semantically equivalent now, but defeats the stated goal of a canonical single check.

Fix: Migrate this callsite to isRemoteAssistance for consistency.


W-5 — uspAuthReadyProvider API design contradiction — High [both reviewers]

lib/providers/auth/usp_auth_ready_provider.dart:5-24

The provider is documented as "Single source of truth for USP auth readiness" yet its own docstring warns "read this at the decision point (ref.read)" because UspClient.isAuthenticated does not trigger Riverpod invalidation. The orchestrator comment at dashboard_orchestrator.dart:132 explicitly says it bypasses this provider for that reason. A provider that cannot be safely ref.watch-ed but is named as SSOT is a trap — future consumers who ref.watch it will get a stale cache on restoreSession.

Fix: Either (a) convert to an extension getter AuthState.isUspReady and remove the Provider wrapper; or (b) rename to uspLoginIntentProvider with clear scope docs that it only reflects loginType intent, not transport-layer state.


W-6 — RemoteAssistanceSessionGuard._checkAndRestoreSession has no test coverage — High [single reviewer B]

lib/page/remote_assistance/views/remote_assistance_session_guard.dart:63

if (ref.read(authProvider).value?.isRemoteAssistance ?? false) return;

This is the one migration in this PR that has no corresponding test. PR adds RA bypass tests for both notifiers and a full usp_auth_ready_provider_test.dart, but RemoteAssistanceSessionGuard has no test file at all. This callsite is the early-return guard preventing credential restore in RA mode — a regression here would be a correctness bug.

Fix: Add test/page/remote_assistance/views/remote_assistance_session_guard_test.dart covering: (1) RA mode → _checkAndRestoreSession returns early; (2) local mode → credential restore proceeds.


W-7 — uspWifiSettingsServiceProvider force-unwraps nullable uspClientProvider — Med [single reviewer A]

lib/page/wifi_settings/providers/usp_wifi_settings_provider.dart:17-18

final uspWifiSettingsServiceProvider = Provider<UspWifiSettingsService>(
  (ref) => UspWifiSettingsService(ref.read(uspClientProvider)!),  // force-unwrap
);

On non-Web platforms or before UspClient is registered in getIt, uspClientProvider returns null. The ! throws a bare Null check operator used on a null value — not a typed ServiceError — so the notifier's on ServiceError catch won't catch it. Compare with uspInternetSettingsServiceProvider which explicitly throws ServiceNotInitializedError.

Fix: Replace ! with an explicit null check:

(ref) {
  final usp = ref.read(uspClientProvider);
  if (usp == null) throw const ServiceNotInitializedError(detail: 'UspClient not available');
  return UspWifiSettingsService(usp);
}

W-8 — New provider missing barrel export — High [single reviewer B]

lib/providers/auth/_auth.dart:1-2

// _auth.dart (current)
//GENERATED BARREL FILE
export 'auth_provider.dart';

usp_auth_ready_provider.dart was added to the same lib/providers/auth/ directory but is not exported from the barrel file. Any future consumer following the barrel convention won't find it without knowing its exact path.

Fix: Add export 'usp_auth_ready_provider.dart'; to _auth.dart.

✅ What looks good
  • Core fix is correct: removing usp.isAuthenticated gate from performFetch unblocks RA sessions without affecting local-login paths.
  • AuthState.isRemoteAssistance getter: well-documented, correctly derived, used consistently in session_guard, router_provider, and dashboard_orchestrator.
  • uspAuthReadyProvider logic: semantically correct; the staleness caveat is clearly documented. Dashboard orchestrator bypass is architecturally sound and well-commented.
  • Test coverage (main paths): RA bypass regression tests for both notifiers, usp_auth_ready_provider_test.dart (5 cases), isRemoteAssistance unit test, null-USP error path — all well-structured.
  • uspMutationLockProvider.withLock() discipline preserved in both performSave() implementations.
  • No hardcoded secrets or injection vectors.

Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.

@AustinChangLinksys AustinChangLinksys added the need-decide Automated review left findings needing Austin to decide (fix/defer/dismiss). label Jul 13, 2026
…#1122 review W-4)

The app could identify the login *kind* (isRemoteAssistance) but had no
canonical "is the user logged in" check — that was scattered as inline
`loginType == LoginType.none` comparisons across 8 sites. Adding isLoggedIn
completes the pair: isLoggedIn answers "logged in?", isRemoteAssistance
answers "which kind?".

- Add AuthState.isLoggedIn => loginType != LoginType.none, named to avoid
  confusion with the transport-layer usp.isAuthenticated (WASM flag).
- Migrate the none-checks in app, router redirect, connection state, top bar,
  login view, root container and general settings widget to isLoggedIn.
- One router site (redirectLogic) keeps the loginType local because it reuses
  the enum value later, not just the logged-in boolean.
- Add isLoggedIn truth-table test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AustinChangLinksys and others added 2 commits July 13, 2026 16:56
…rtcuts

Document that AuthState.loginType is the single three-state source of truth and
that isLoggedIn / isRemoteAssistance are derived named shortcuts, not a separate
mechanism — so readers know when to use the enum vs the booleans.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes the pre-existing prefer_const_literals_to_create_immutables hint on the
empty-map fromJson case, now that this file is touched by the auth getter tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@AustinChangLinksys

Copy link
Copy Markdown
Collaborator Author

Review follow-up — investigation results & fixes

Verified each item on the branch (checked out at the reviewed commit) before acting. Summary below; new commits pushed.

W-1 — uspAuthReadyProvider has no consumer → fixed (removed)

Confirmed the provider had zero production consumers — nothing under lib/ referenced it except one explanatory comment. It was introduced as a "single source of truth" but the actual fix removed the fetch gate (relying on the router guard) instead of routing through it, leaving it dead code.

Beyond being unused, keeping it would push the auth concern back into page providers (each page would ref.read it). Auth is a navigation-layer concern: the router already gates every /usp route on loginType (RA-aware), so pages render data without an auth dependency — matching the 9 sibling USP settings providers that never had a gate.

→ Deleted usp_auth_ready_provider.dart + its test (a74e62c).

W-2 / W-3 — performSave() has no RA write guard → acknowledged, out of scope

The observation is real (nothing in the notifier → service → UspClient → WASM path blocks a write in RA mode), but it is pre-existing and not introduced by this PR:

  • The removed !usp.isAuthenticated fetch gate was never the thing protecting the write path — it only happened to hide these two pages from RA. The 9 sibling USP settings providers (dmz, local_network, firewall, port_forwarding, static_routing, ipv6_port_service, dhcp) never had such a gate, so RA could already reach performSave() on all of them. This PR just makes wifi/internet consistent with that existing behavior.
  • The mitigation cited in the review (isRemoteReadOnly, RouterRepository JNAP write-blocking, checkRemoteReadOnly, the remote-read-only-mode doc) does not exist in this codebase — it describes the older JNAP architecture that was removed during the USP/WASM migration. There is no such layer to rely on for USP.

There is currently no product requirement defining which features should be read-only in remote mode. RA write-protection, if desired, should be a separate change covering all settings pages (enforced at the UspClient mutation layer or via a shared RA-aware bottom bar), not bolted onto this #1119 fix. Not addressing it here.

W-4 — loginType migration is partial → fixed (completed the model)

The app could identify the login kind (isRemoteAssistance) but had no canonical "is the user logged in" check — that was scattered as inline loginType == LoginType.none comparisons across 8 sites.

→ Added AuthState.isLoggedIn (loginType != LoginType.none), named to avoid confusion with the transport-layer usp.isAuthenticated (WASM flag). Migrated 7 of the 8 sites (app, router redirect, connection state, top bar, login view, root container, general settings) to isLoggedIn. One router site (redirectLogic) keeps the raw loginType because it reuses the enum value later, not just the boolean. (04d0e44)

Model is now: loginType is the single three-state source of truth; isLoggedIn / isRemoteAssistance are derived named shortcuts — documented on the field (d957f23). Read the enum directly only when the full three-state value is needed (transition detection, branching on local).

Suggestions

  • Formatting (6-space indent comment): Not reproducible — dart format --set-exit-if-changed reports 0 changes on all touched files. No change.
  • S-2 (orchestrator ref.listen only invalidates on LoginType.local) / S-4 (session guard uses two RA mechanisms): Both are pre-existing and untouched by this PR. S-4 is intentional: build() uses the compile-time GlobalConfig.remote.isActive ("is this an RA build") while _checkAndRestoreSession uses the runtime isRemoteAssistance ("is this session RA") — different questions.

Verification

dart format clean (0 changed), flutter analyze clean on all changed files, full functional suite ./run_tests.sh — 3265 passed.

@AustinChangLinksys AustinChangLinksys left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated Review — Round 3 · 69eb8f1..2cdbe91 (full)

Verdict: 💬 Self-review (comment only) — Austin's own PR; GitHub prohibits self-approve. Full review for reference.

Prior Round 2 review (at SHA a79aded) remains on record. This round re-confirms still-open items and adds new findings.

Conf. Where Issue (one-liner)
⚠️ 🟢High usp_internet_settings_notifier.dart:139 [both reviewers, prior] performSave() zero RA-mode guard — still unfixed
⚠️ 🟢High usp_wifi_settings_provider.dart:174 + usp_wifi_settings_service.dart:18 [both reviewers, prior] performSave() no RA guard + force-unwrap ! = crash on null USP — still unfixed
⚠️ 🟢High usp_auth_ready_provider.dart:19 [both reviewers, prior] uspAuthReadyProvider has zero production consumers (dead code) — still unfixed
⚠️ 🟢High remote_assistance_session_guard.dart:63-75 [single B, prior] _checkAndRestoreSession still has no test coverage — still unfixed
⚠️ 🟢High usp_wifi_settings_service.dart:18 [single A, prior] Force-unwrap ! on uspClientProvider — crashes instead of ServiceNotInitializedError — still unfixed
⚠️ 🟢High lib/providers/auth/_auth.dart:2 [single B, prior] usp_auth_ready_provider.dart not exported from barrel — still unfixed
⚠️ 🟢High auth_state.dart [both reviewers, new] isLoggedIn getter stated in PR but absent from codebase; ~5 call-site migrations not committed
⚠️ 🟢High app.dart:236 [single A, new] SSE resume _tryResumeSse() still uses raw loginType pattern
💡 🟡Med router_provider.dart:221 [single B, new] Intentional raw loginType at line 221 undocumented — future reader won't understand why

Confidence: 🟢High = code-verified · 🟡Med = located + reasoned, not fully confirmed · ⚪Low = speculative, please double-check.
Items marked [both reviewers] were independently flagged by two agents → higher confidence.

✅ Fixed since prior rounds
  • W-4 FIXED ✓router_provider.dart:143-145 Remote-build-mode branch correctly migrated to isRemoteAssistance getter.
  • W-5 FIXED ✓uspAuthReadyProvider API design contradiction resolved: usp_auth_ready_provider.dart now has full docstring explaining RA-mode staleness caveat; orchestrator bypass comment consistent.
  • Core fix correct ✓ — Removing !usp.isAuthenticated gate from performFetch() in both Internet/WiFi notifiers is correct. The router guards routes; usp == null guard covers uninitialized case. Nine sibling providers never had this gate.
  • W-7 (partial) ✓performFetch() now has usp == null null guard before reaching uspWifiSettingsServiceProvider, preventing the crash on the fetch path. (But performSave() still lacks this guard — see table.)
  • isRemoteAssistance getter ✓ — Correctly derived, well-documented, used consistently in session_guard, router_provider:143, dashboard_orchestrator.
⚠️ Warning Details

W-1 (prior, still open) — performSave() no RA-mode guard (Internet Settings) — 🟢High [both reviewers]

lib/page/internet_settings/providers/usp_internet_settings_notifier.dart:139-165

@override
Future<void> performSave() async {
  // ...no isRemoteAssistance check...
  // ...no usp == null check...
  try {
    final service = ref.read(uspInternetSettingsServiceProvider);
    await ref.read(uspMutationLockProvider).withLock(() async {
      await service.saveAll(...);   // ← proceeds in RA mode

RA user can enter edit mode (fetch now succeeds) and trigger a write that goes through to WASM/USP, changing WAN settings without local-admin authorization.

Fix:

if (ref.read(authProvider).value?.isRemoteAssistance ?? false) return;

Add at top of performSave() body.


W-2 (prior, still open) — performSave() no RA-mode guard + force-unwrap crash (Wi-Fi Settings) — 🟢High [both reviewers]

lib/page/wifi_settings/providers/usp_wifi_settings_provider.dart:174 + usp_wifi_settings_service.dart:18

// Service provider (usp_wifi_settings_service.dart:18) — UNCHANGED:
final uspWifiSettingsServiceProvider = Provider<UspWifiSettingsService>(
  (ref) => UspWifiSettingsService(ref.read(uspClientProvider)!),  // force-unwrap
);

// performSave() (usp_wifi_settings_provider.dart:174-199) — no guard:
@override
Future<void> performSave() async {
  try {
    await ref.read(uspMutationLockProvider).withLock(() async {
      await _svc.saveQuickSetup(...);   // ← _svc = ref.read(uspWifiSettingsServiceProvider)

_svc calls uspWifiSettingsServiceProvider which force-unwraps uspClientProvider!. If client is null at save time, this throws Null check operator used on a null value — an unhandled crash, not a typed ServiceError. performFetch() has a usp == null guard but performSave() does not.

Fix: (1) Replace ! in service provider with explicit null check (throw ServiceNotInitializedError). (2) Add isRemoteAssistance guard at top of performSave().


W-3 (prior, still open) — uspAuthReadyProvider zero production consumers — 🟢High [both reviewers]

lib/providers/auth/usp_auth_ready_provider.dart:19

Codebase search (lib/**/*.dart) returns zero import/usage hits for uspAuthReadyProvider outside its own file. The only code reference is dashboard_orchestrator.dart:132 — a comment explaining why NOT to use it. The provider is well-documented and tested but dead at runtime. Either wire it into performFetch paths (replacing their duplicated inline logic) or document as "reserved / future use."


W-6 (prior, still open) — _checkAndRestoreSession no test coverage — 🟢High [single reviewer B]

lib/page/remote_assistance/views/remote_assistance_session_guard.dart:63-75

Search of test/ returns zero results for any remote_assistance* test file. This is the sole migration callsite in this PR without a test. Covers three paths: RA-bypass early return, null-credentials early return, credential restore.

Fix: Add test/page/remote_assistance/views/remote_assistance_session_guard_test.dart covering at minimum the RA-bypass case.


W-7 (prior, still open) — uspWifiSettingsServiceProvider force-unwrap crash — 🟢High [single reviewer A]

lib/page/wifi_settings/services/usp_wifi_settings_service.dart:18

final uspWifiSettingsServiceProvider = Provider<UspWifiSettingsService>(
  (ref) => UspWifiSettingsService(ref.read(uspClientProvider)!),  // ← crash
);

Contrast with uspInternetSettingsServiceProvider (same file direction, different service) which throws ServiceNotInitializedError. The Wi-Fi service crashes with bare Null check operator — unhandled, not typed, bypasses on ServiceError catch.

Fix:

(ref) {
  final usp = ref.read(uspClientProvider);
  if (usp == null) throw const ServiceNotInitializedError(detail: 'UspClient not available');
  return UspWifiSettingsService(usp);
}

W-8 (prior, still open) — usp_auth_ready_provider.dart missing barrel export — 🟢High [single reviewer B]

lib/providers/auth/_auth.dart:2

//GENERATED BARREL FILE
export 'auth_provider.dart';
// ← usp_auth_ready_provider.dart not exported

Any future consumer following the barrel convention won't find it.

Fix: Add export 'usp_auth_ready_provider.dart'; to _auth.dart.


N-1 (new) — isLoggedIn getter promised by PR but absent from codebase — 🟢High [both reviewers]

lib/providers/auth/auth_state.dart

PR description and diff state bool get isLoggedIn => loginType != LoginType.none; was added. Actual disk inspection: isLoggedIn is absent from auth_state.dart. Codebase-wide grep returns zero matches. ~5 call sites remain on raw loginType comparisons:

  • app.dart:236: loginType == null || loginType == LoginType.none
  • root_container.dart:48: loginType == LoginType.none
  • login_local_view.dart:110: loginType != null && loginType != LoginType.none
  • usp_top_bar.dart:74: same pattern
  • router_provider.dart:162-165: loginType == null || loginType == LoginType.none

Either commit the getter + consumer migrations, or scope them out explicitly with a follow-up ticket.


N-2 (new) — _tryResumeSse() raw loginType pattern — 🟢High [single reviewer A]

lib/app.dart:236

void _tryResumeSse() {
  final loginType = ref.read(authProvider).value?.loginType;
  if (loginType == null || loginType == LoginType.none) return;

This is one of the ~5 sites that should migrate to isLoggedIn per PR intent. Not a functional bug (semantically equivalent), but defeats the stated unification goal.

Cross-reviewed by two independent agents (security+correctness / architecture+maintainability). Automated — please sanity-check before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

need-decide Automated review left findings needing Austin to decide (fix/defer/dismiss).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The Wi-Fi Settings in the Remote Assistance UI cannot be operated.

1 participant